Skip to content

feat(ENG-1084): defender hardening, detect-and-gate refactor + review fixes#73

Open
hiskudin wants to merge 10 commits into
mainfrom
fix/defender-review-hardening
Open

feat(ENG-1084): defender hardening, detect-and-gate refactor + review fixes#73
hiskudin wants to merge 10 commits into
mainfrom
fix/defender-review-hardening

Conversation

@hiskudin

@hiskudin hiskudin commented Jul 3, 2026

Copy link
Copy Markdown
Collaborator

Summary

Addresses the findings from a full review of the package. The centerpiece is an architectural change: Defender no longer mutates tool-result content. It now detects, scores, and gatesresult.sanitized returns the original content (optionally boundary-wrapped) and allowed: false is the block signal. Content rewriting (role-stripping, [REDACTED], [ENCODED DATA], [CONTENT BLOCKED]) was trivially bypassable, corrupted legitimate data, and was where almost every bug lived — so removing it fixes several findings by construction and deletes a large, buggy surface.

7 focused commits, each independently green (tests + typecheck + lint + build). 353 → 313 tests (the drop is the deletion of tests covering removed mutation code; net new regression tests were added throughout).

What's in it

Commit Fixes Notes
detect-and-gate (foundation) sanitized returns original content; detections sourced from the detector; boundary annotation kept
ReDoS C1 (Critical) Linearized the Morse regex + per-field analysis cap + size-metrics fix. A 200k-char field went from ~30s of blocked event loop to <2s
Tier 2 availability H2 result.tier2Available flag + warn-once + opt-in requireTier2 fail-closed. Missing peer deps no longer silently disable ML defense
key detection + arrays H1, H6 Object keys are now scanned (injection in a key used to pass with allowed:true); large arrays no longer silently drop items; coverageDegraded flag added
risk reporting M7, M15, L1–L3 riskLevel now spans low..critical; classify() NaN guard; removed the blockHighRisk split-brain
packaging M9, M10, M11 Bundler-safe optional-peer imports; per-condition ESM types; dual-package-safe Tier 3 registry. Also un-broke SFE, which never actually loaded (its dynamic-import trick always threw)
dead-code + docs M18 + cleanup Removed the now-unreachable mutation path (−1,239 lines); doc accuracy fixes

Breaking changes

  • result.sanitized is no longer redacted/normalized — it's the original content (optionally boundary-wrapped). Consumers must gate on result.allowed rather than relying on rewritten output.
  • riskLevel now starts at low (was medium) and can reach critical. It remains diagnostic-only; block logic is unchanged.
  • Removed internal mutation utilities (Sanitizer, removePatterns, etc.) — none were exported from the package entry, so this is non-breaking for consumers importing @stackone/defender.

Deferred

Phase 5 (evasion hardening) — S7 role-marker anchoring, chunk-truncation/density/pattern-gap fixes — is intentionally not here. Those change detection behavior and must be measured against the FP/FN benchmark datasets before shipping, so they belong in a separate benchmark-gated PR.

Verification

npm test (313 passing), npm run test:typecheck, npm run lint, and npm run build all clean. New regression tests cover: the ReDoS guard, content-preservation-while-gating, key-injection detection, the Tier 2 fail-open/fail-closed paths, large-array preservation, and the risk-level range.

🤖 Generated with Claude Code

hiskudin and others added 7 commits July 2, 2026 11:42
…riting it

Defender no longer mutates tool-result content. defendToolResult().sanitized
now returns the original value (optionally boundary-wrapped); role-marker
stripping, pattern/encoding redaction, and in-place [CONTENT BLOCKED]
replacement are removed from the returned payload. Blocking is expressed
solely via allowed: false; detection metadata (detections/fieldsSanitized/
patternsByField) is now sourced directly from the detector, so every detected
pattern is reported (fixes previously-dropped medium-severity matches).

Boundary annotation and all Tier 1/2/3 detection/scoring are unchanged.

BREAKING CHANGE: result.sanitized is no longer redacted/normalized. Consumers
must gate on result.allowed rather than relying on rewritten content.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The Tier 1 encoding detector runs on every risky field with no length cap,
and its Morse regex `(?:[.-]+[ ]){4,}[.-]+` backtracked catastrophically on
long dot/dash runs (200k dots ≈ 30s of blocked event loop). A remotely
supplied email/document body could freeze the host process.

- Linearize the Morse regex by bounding each symbol group to {1,8} (longer
  runs are not valid Morse anyway).
- Bound the markdown_hidden_instruction regex with negated char classes
  instead of lazy `.*?` spans (defense-in-depth; it was already capped by the
  detector's 50k input limit).
- Add `maxFieldAnalysisLength` (default 50000): cap the text Tier 1 runs heavy
  regex/encoding detection over; flag `metadata.analysisTruncated` past the cap.
- Fix the size-metrics bypass: risky-field strings now count toward the
  traversal `maxSize` budget (they previously skipped updateSizeMetrics).

Regression test: a 200k-char dot field now completes in <2s (was ~30s).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Tier 2 is on by default, but onnxruntime-node / @huggingface/transformers are
optional peer deps. Previously, if they were missing the model load failure was
swallowed to a per-string skipReason with no top-level signal — consumers who
set blockHighRisk believed they had ML defense but silently ran Tier 1 only.

- Add `result.tier2Available` — set to `false` when Tier 2 is enabled but the
  model/runtime can't load, so monitoring can alert on degraded ML defense.
- Warn once per instance (was: once per failed call — removed the per-call
  warn in onnx-classifier.loadModel; the caller now owns messaging).
- Add `requireTier2` option: when true, defendToolResult and warmupTier2 throw
  instead of degrading (fail-closed).
- warmupTier2 now fails open by default (warn) rather than throwing, matching
  the lazy path; it throws only under requireTier2.
- README: add a Requirements section documenting the peer deps + requireTier2.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…rray data (H1, H6)

- H1: object KEYS are now scanned by the Tier 1 detector (detect-only — keys
  are never rewritten, which would change the object shape). An injection
  hidden in a key now contributes to detections/risk/allowed; previously keys
  were copied verbatim and invisible to every tier.
- H6: large arrays (> largeArrayThreshold) no longer drop items. The previous
  behavior returned only the first 100 items plus a notice, silently losing the
  rest of the payload. Now all items are returned; only Tier 1 detection is
  capped to the first 100, and coverage is flagged.
- Add result.coverageDegraded: true when Tier 1 detection coverage was reduced
  (field over maxFieldAnalysisLength, large-array partial scan, or a depth/size
  traversal limit). Content is always returned in full.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- M7: remove the now-inert blockHighRisk from the sanitizer layer (detect-and-gate
  made it dead). The only remaining flag is PromptDefenseConfig.blockHighRisk,
  resolved once; config:{blockHighRisk} and the blockHighRisk option now gate
  identically.
- M15: guard Tier2Classifier.classify() against a NaN logit (matches the batch
  paths) so a broken model output isn't silently treated as benign.
- L1: overallRiskLevel now propagates a per-field critical (via a raiseOverallRisk
  max-helper) instead of capping at high; cumulative escalation raises rather than
  overwrites.
- L3: riskLevel floor is now low (was medium), so benign payloads report low and the
  full low..critical range is reachable. riskLevel is diagnostic only; blocking
  logic is unchanged.
- L2 (already fixed in Phase 0): regression test that a medium-severity match appears
  in detections.
- README: update the risk-level section/table (starts at low; drop stale
  stripped/redacted language).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…er 3 registry (M9/M10/M11)

- M9: load the optional peers (onnxruntime-node, @huggingface/transformers) via a
  shared dynamicImport() helper that passes a runtime specifier to the real
  import() operator, so bundlers (webpack/esbuild) don't try to resolve them at
  build time. Refactor the SFE fasttext.wasm load onto the same helper — its old
  `new Function("return import(spec)")` trick threw "A dynamic import callback was
  not specified" and silently failed open, so SFE never actually loaded; it works
  now.
- Because the peers are no longer statically imported, tsc lost the transitive
  @types/node; declare @types/node explicitly and add "node" to tsconfig types.
- M10: per-condition types in the exports map so ESM consumers resolve
  dist/index.d.mts instead of the CJS-flavored .d.cts.
- M11: back the Tier 3 provider registry with a globalThis Symbol slot so an app
  that loads both the CJS and ESM builds shares one registry (a module-level
  singleton gave each build its own, so a registered provider was invisible to the
  other realm).

Verified: build emits no literal peer specifiers in dist; dist/index.d.mts present
and wired; 353 tests + typecheck + lint green.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Detect-and-gate (Phase 0) unwired the entire content-mutation path. This removes
the now-unreachable code — traced to spec-only usage; none of it is exported from
the package entry, so it's non-breaking for consumers and was already tree-shaken
out of the shipped bundle.

Deleted (whole files, the mutation path):
- src/sanitizers/sanitizer.ts (Sanitizer, createSanitizer, sanitizeText, suggestRiskLevel)
- src/sanitizers/pattern-remover.ts (removePatterns + variants)
- src/sanitizers/role-stripper.ts (stripRoleMarkers, containsRoleMarkers, findRoleMarkers)

Trimmed dead exports from files that stay:
- encoding-detector: redactAllEncoding, decodeAllEncoding, containsEncodedContent
- normalizer: containsSuspiciousUnicode, analyzeSuspiciousUnicode
- boundary: generateXMLBoundary

Docs (M18): fix the tier3_only line (T1 detects, doesn't strip) and the Risky Field
Detection section (clarify Tier 1 value scoping vs Tier 2 scan-all + key scanning).

Specs updated to drop deleted-symbol tests; detection coverage (normalizer / leet /
encoding-detection) retained. 313 tests + typecheck + lint + build green.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 3, 2026 12:11
@hiskudin hiskudin requested a review from a team as a code owner July 3, 2026 12:11

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens Defender by completing the “detect-and-gate” refactor: tool-result content is no longer rewritten/redacted, and instead Defender detects/scorers threats and gates forwarding via allowed (with optional boundary annotation). It also incorporates review-driven security/performance fixes (notably ReDoS guards), improved Tier 2 availability signaling, and packaging/doc/test updates to match the new model.

Changes:

  • Refactors Tier 1 from mutation to detection-only: preserve original content in result.sanitized while recording detections/risk and gating via allowed.
  • Adds ReDoS/DoS protections (bounded Morse + markdown-link regex, per-field analysis cap) and expands Tier 1 coverage (object key scanning, large-array handling + degraded-coverage signaling).
  • Makes Tier 2 optional-peer dependency failures explicit (tier2Available, warn-once, requireTier2 fail-closed), adds bundler-safe dynamic imports, and updates exports/types + docs/tests.

Reviewed changes

Copilot reviewed 24 out of 26 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
tsconfig.json Adds Node types for build/test typing support.
src/utils/dynamic-import.ts Bundler-resistant helper for optional peer dependency loading.
src/utils/boundary.ts Removes XML boundary helper; keeps UD boundary wrapping path.
src/types.ts Adds analysisTruncated metadata to surface capped Tier 1 coverage.
src/sfe/preprocess.ts Switches optional peer import to dynamicImport for bundler safety.
src/sanitizers/sanitizer.ts Removes mutation-based composite sanitizer implementation.
src/sanitizers/role-stripper.ts Removes role-stripping mutator utilities (now detection-only model).
src/sanitizers/pattern-remover.ts Removes pattern redaction utilities (now detection-only model).
src/sanitizers/normalizer.ts Removes unused suspicious-unicode analysis helpers.
src/sanitizers/index.ts Stops exporting removed mutation utilities; keeps analysis helpers needed by detectors.
src/sanitizers/encoding-detector.ts ReDoS-hardens Morse detection and trims mutation-oriented helpers.
src/core/tool-result-sanitizer.ts Implements detect-and-gate, analysis capping, key scanning, large-array behavior, and risk aggregation changes.
src/core/prompt-defense.ts Updates API semantics/docs, Tier 2 availability handling (tier2Available, requireTier2), and coverageDegraded reporting.
src/config.ts Adds default per-field analysis cap constant for Tier 1.
src/classifiers/tier3-orchestrator.ts Makes Tier 3 provider registry dual-package-safe via globalThis Symbol slot.
src/classifiers/tier2-classifier.ts Guards against NaN/non-finite logits in single-text classify path.
src/classifiers/patterns.ts ReDoS-hardens a markdown-link injection regex (bounded, linear).
src/classifiers/onnx-classifier.ts Uses dynamicImport for optional peer deps; reduces repeated warning noise on load failure.
specs/utils.spec.ts Updates tests after XML boundary removal.
specs/tier3.spec.ts Adds regression test for global Symbol provider registry behavior.
specs/tier2-availability.spec.ts Adds fail-open/fail-closed coverage for Tier 2 unavailability.
specs/sanitizers.spec.ts Removes tests for deleted mutation utilities; retains detector/normalizer coverage.
specs/integration.spec.ts Updates expectations to “preserve content + gate” and adds ReDoS/coverage regression tests.
README.md Updates public contract/docs to emphasize detect-and-gate and Tier 2 availability semantics.
package.json Refines conditional exports/types mapping and adds @types/node.
package-lock.json Lockfile updates for dependency graph changes.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/core/tool-result-sanitizer.ts Outdated
@hiskudin hiskudin changed the title Harden defender: detect-and-gate refactor + review fixes feat(ENG-1084): arden defender: detect-and-gate refactor + review fixes Jul 3, 2026
`npm install --save-dev @types/node` in the packaging commit bumped @types/node
(25.3.3→25.9.4), removed @emnapi/core + @emnapi/runtime, and bumped an unrelated
transitive — none of which was intended. Revert package-lock.json to match main
and drop the @types/node devDependency.

Node types for the typecheck come from the already-transitive @types/node via the
`tsconfig types: ["node"]` entry added in Phase 4, so no declared dep or lockfile
change is needed. typecheck + 313 tests + lint + build stay green.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@hiskudin hiskudin changed the title feat(ENG-1084): arden defender: detect-and-gate refactor + review fixes feat(ENG-1084): harden defender: detect-and-gate refactor + review fixes Jul 3, 2026
@hiskudin hiskudin changed the title feat(ENG-1084): harden defender: detect-and-gate refactor + review fixes feat(ENG-1084): harden defender, detect-and-gate refactor + review fixes Jul 3, 2026
…R review)

Copilot flagged that large-array items past the detection scan limit were
returned verbatim (`return item`), bypassing not just Tier 1 detection but also
prototype-pollution key stripping (DANGEROUS_KEYS), depth/size limits, and
boundary wrapping — a dangerous key could hide in a tail element and reach the
caller unprocessed.

Thread a `detect` flag through the traversal: tail items are still fully
traversed for structural protections but skip only the expensive per-string
Tier 1 analysis. Data is still never dropped; reduced detection coverage stays
flagged via `analysisTruncated`.

Regression test: a __proto__ key in a tail item (index 1400, past the 100-item
scan limit) is still stripped.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@hiskudin hiskudin changed the title feat(ENG-1084): harden defender, detect-and-gate refactor + review fixes feat(ENG-1084): defender hardening, detect-and-gate refactor + review fixes Jul 6, 2026
@hiskudin hiskudin requested a review from Copilot July 7, 2026 08:29

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 24 out of 25 changed files in this pull request and generated 2 comments.

Comment thread src/types.ts Outdated
Comment thread src/classifiers/tier2-classifier.ts Outdated
…output

Addresses two PR review comments:
- types.ts: the `analysisTruncated` docstring only mentioned the per-field
  length cap, but the flag is also set when a large array is partially scanned.
  Broaden the doc so observability consumers aren't misled.
- Tier2Classifier.classify(): a non-finite logit (NaN/Infinity) set score 0,
  which yields confidence 1.0 (max) — a broken inference looked like a
  max-confidence benign classification. Report it as skipped:true with a
  skipReason instead of a fake confident-benign result.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants